Skip to content

fix(lookup): skip empty cells in approximate MATCH/VLOOKUP/HLOOKUP/XLOOKUP (HF-223)#1697

Open
marcin-kordas-hoc wants to merge 3 commits into
developfrom
task/hf-223-match-empty-cells
Open

fix(lookup): skip empty cells in approximate MATCH/VLOOKUP/HLOOKUP/XLOOKUP (HF-223)#1697
marcin-kordas-hoc wants to merge 3 commits into
developfrom
task/hf-223-match-empty-cells

Conversation

@marcin-kordas-hoc

@marcin-kordas-hoc marcin-kordas-hoc commented Jun 22, 2026

Copy link
Copy Markdown
Collaborator

What & why

Approximate MATCH, VLOOKUP, HLOOKUP and XLOOKUP returned #N/A (or, in descending mode, the wrong position) when the sorted search range contained genuinely empty cells. Excel 2021 and Google Sheets skip empty cells when computing the lower/upper bound; HyperFormula instead landed on an empty cell during binary search (its EmptyValue Symbol never matched the key). [HF-223]

Root cause

findLastOccurrenceInOrderedRange (src/interpreter/binarySearch.ts): compare() ranks EmptyValue below every value, breaking the sort invariant the binary search relies on, and the typeof foundValue !== typeof searchKey guard then turned a landing-on-empty into #N/A. Shared by approximate MATCH(±1), sorted VLOOKUP/HLOOKUP, and XLOOKUP(searchMode ±2).

Fix

  • The ordered search now runs over a compacted list of non-empty cell indices and maps the result back to the original index space, so empty cells keep their slots and the matched non-empty cell's original 1-based position is reported unchanged.
  • AdvancedFind.findNormalizedValue skips EmptyValue on its in-memory ordered path for the same reason.
  • Empty strings are unaffected (text ranks above numbers, so they still terminate a numeric run). Exact match (matchType 0) is untouched.

Performance & edge cases

  • Complexity: the empty-skip pre-scan is O(n) over the search range, trading away binary search's O(log n) guarantee. This is required for correctness — with empty cells interspersed the search predicate is no longer monotonic, so the binary search cannot run directly on the original range (rationale documented inline in binarySearch.ts). A future optimization can keep O(log n) when the range is statically known to be empty-free.
  • All-empty range: returns NOT_FOUND directly instead of falling through to the lower/upper-bound edge-case logic (which could otherwise return offset 0).
  • Approximate-bound stepping: the "next" position steps to the next non-empty index, not foundIndex + 1, so skipped empty slots never shift the reported position.

Excel / Google Sheets parity

Behaviour verified against the latest Excel and Google Sheets, including exact-vs-blank, empty-string-not-skipped, and the descending early-stop case.

Tests

Public test suite: handsontable/hyperformula-tests#17 (matching branch task/hf-223-match-empty-cells).

Definition of Done

  • Production code + JSDoc
  • Tests (hyperformula-tests#17, matching branch)
  • Changelog
  • i18n — N/A (shared-logic fix, no function add/rename)
  • Docs — N/A (was a bug, not a documented limitation)

Note

Medium Risk
Touches shared ordered-search logic used by multiple lookup functions; behavior change is intentional but could affect formulas that relied on the old wrong results. Exact-match path is explicitly isolated to reduce regression risk.

Overview
Fixes approximate MATCH, VLOOKUP, HLOOKUP, and XLOOKUP when the sorted search range has genuinely empty cells—they no longer return #N/A or the wrong position because binary search treated EmptyValue as ordered data.

findLastOccurrenceInOrderedRange now skips blanks only for lower/upper-bound modes: it builds a compact list of non-empty indices, runs the search there, and maps the result back so reported positions still match the original range. Exact match (returnNotFound) stays on the original range with prior semantics. All-empty ranges return not found; stepping to the “next” bound uses the next non-empty index, not foundIndex + 1.

AdvancedFind.findNormalizedValue applies the same empty-cell skip on its in-memory approximate path. Empty strings are unchanged; changelog documents the fix ([#1697]).

Reviewed by Cursor Bugbot for commit dde3c8a. Bugbot is set up for automated code reviews on this repo. Configure here.

@netlify

netlify Bot commented Jun 22, 2026

Copy link
Copy Markdown

Deploy Preview for hyperformula-dev-docs ready!

Name Link
🔨 Latest commit dde3c8a
🔍 Latest deploy log https://app.netlify.com/projects/hyperformula-dev-docs/deploys/6a3d047d7e1d060008e616d5
😎 Deploy Preview https://deploy-preview-1697--hyperformula-dev-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit c79e10c. Configure here.

Comment thread src/interpreter/binarySearch.ts
@github-actions

github-actions Bot commented Jun 22, 2026

Copy link
Copy Markdown

Performance comparison of head (dde3c8a) vs base (72205bd)

                                     testName |   base |   head | change
------------------------------------------------------------------------
                                      Sheet A | 495.82 |  492.5 | -0.67%
                                      Sheet B | 157.76 | 157.32 | -0.28%
                                      Sheet T | 139.53 |    142 | +1.77%
                                Column ranges | 462.33 |  467.5 | +1.12%
Sheet A:  change value, add/remove row/column |  14.98 |  15.89 | +6.07%
 Sheet B: change value, add/remove row/column | 127.89 | 129.87 | +1.55%
                   Column ranges - add column | 143.06 | 146.25 | +2.23%
                Column ranges - without batch | 432.31 | 457.66 | +5.86%
                        Column ranges - batch | 113.51 | 112.41 | -0.97%

…223)

Excel and Google Sheets ignore genuinely empty cells (but not empty
strings) when computing the lower/upper bound for an approximate match.
HyperFormula instead landed on an empty cell during binary search and
returned #N/A (its EmptyValue Symbol never matched the key), or, in
descending mode, reported the wrong position.

findLastOccurrenceInOrderedRange now runs the ordered search over a
compacted list of non-empty cell indices and maps the result back to the
original index space, so empty cells keep their slots and the matched
non-empty cell's original 1-based position is reported unchanged. When
the range has no non-empty cells the function returns NOT_FOUND, so an
all-empty range yields #N/A for every direction/bound instead of falling
through to the offset-0 branches. The in-memory ordered path in
AdvancedFind.findNormalizedValue skips EmptyValue for the same reason.
Empty strings are unaffected (text is ranked above numbers, so they
still terminate a numeric run). Exact match (matchType 0) is untouched.

Shared by approximate MATCH(+/-1), sorted VLOOKUP/HLOOKUP, and
XLOOKUP(searchMode +/-2). Tests for the public test suite are tracked
separately in handsontable/hyperformula-tests.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@marcin-kordas-hoc marcin-kordas-hoc force-pushed the task/hf-223-match-empty-cells branch from c79e10c to ed10ce5 Compare June 22, 2026 23:24
…e JSDoc (HF-223)

Address prep-flip review notes: explain why the empty-cell compaction is
O(n) (correctness requires it; empties make the predicate non-monotonic)
and add a JSDoc block to AdvancedFind.findNormalizedValue for family
consistency with findLastOccurrenceInOrderedRange. Comment/JSDoc only —
no behavior change.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@marcin-kordas-hoc marcin-kordas-hoc requested a review from sequba June 23, 2026 08:07
Comment thread src/interpreter/binarySearch.ts
Comment thread CHANGELOG.md Outdated
…HF-223)

Per review: the empty-skipping compaction must apply to approximate (bound) lookups
only. Run exact-match mode (ifNoMatch === 'returnNotFound') directly over the original
range so it keeps its O(log n) guarantee and its pre-empty-skip behaviour — an exact
search neither matches a blank nor is redirected past one. CHANGELOG reworded to the
house style (PR link).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@qunabu

qunabu commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Task linked: HF-223 Fix MATCH incompatibility with Excel

@marcin-kordas-hoc marcin-kordas-hoc requested a review from sequba June 25, 2026 13:44
@codecov

codecov Bot commented Jun 25, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.15%. Comparing base (72205bd) to head (dde3c8a).
⚠️ Report is 1 commits behind head on develop.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1697      +/-   ##
===========================================
- Coverage    97.16%   97.15%   -0.01%     
===========================================
  Files          176      176              
  Lines        15322    15334      +12     
  Branches      3387     3394       +7     
===========================================
+ Hits         14887    14898      +11     
- Misses         435      436       +1     
Files with missing lines Coverage Δ
src/Lookup/AdvancedFind.ts 98.03% <100.00%> (+0.08%) ⬆️
src/interpreter/binarySearch.ts 97.59% <100.00%> (-1.04%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants